Fix RPCtoDTTranslator #25275

Dr15Jones · 2018-11-18T17:53:24Z

Fixed a crash in RPCtoDTTranslator (the first commit)
Also did code improvements to the package, including one case where the orginal code would have lead to a crash.

This avoids a crash seen in production. In addition, moved declaration of additional loop variables to be within the scope of the loop.

The constructor now holds a const& to the collection.

Create a L1MuTMChambPhContainer on the stack rather than having a pointer to null. Also made calling arguments const.

The helper classes were being made each call and where being allocated via a shared_ptr. Putting them on the stack is more efficient. Also tried to avoid some copies of returned values.

Moved the variables to be on the stack. They have the same lifetime as before but can be handled more efficiently.

cmsbuild · 2018-11-18T17:53:42Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-11-18T17:56:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25275/7296

Dr15Jones · 2018-11-18T17:57:33Z

please test

cmsbuild · 2018-11-18T17:58:36Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31721/console Started: 2018/11/18 20:01

cmsbuild · 2018-11-18T17:58:37Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

L1Trigger/L1TTwinMux

@nsmith-, @rekovic, @cmsbuild, @thomreis can you please review it and eventually sign? Thanks.
@Martin-Grunewald this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2018-11-18T17:59:08Z

NOTE: many of the helper classes in this package, e.g. RPCHitCleaner, should just be converted to a function which takes as arguments the values passed to the constructor and return the value accessed from the get* function.

fabiocos · 2018-11-18T18:02:47Z

please test workflow 136.855,136.867,136.879,136.891

Dr15Jones · 2018-11-18T19:39:36Z

L1Trigger/L1TTwinMux/src/DTLowQMatching.cc

@@ -41,15 +41,15 @@ void DTLowQMatching::Matching(int track_seg){

  L1MuDTChambPhDigi * dtts=nullptr;
  L1MuDTChambPhDigi * rpcts1=nullptr;
-  auto m_phiRPCDigis_tm=std::make_shared<L1MuTMChambPhContainer>();


The shared_ptr only had the lifetime of this function.

Dr15Jones · 2018-11-18T19:39:54Z

L1Trigger/L1TTwinMux/src/DTRPCBxCorrection.cc

@@ -41,16 +41,16 @@ void DTRPCBxCorrection::run( const edm::EventSetup& c) {

 void DTRPCBxCorrection::BxCorrection(int track_seg){

-  auto m_phiDTDigis_tm=std::make_shared<L1MuTMChambPhContainer>();


The share_ptr only lived as long as the function.

Dr15Jones · 2018-11-18T19:40:33Z

L1Trigger/L1TTwinMux/src/IOPrinter.cc

-          dtts1=nullptr; dtts2=nullptr;
-          dtts1 = inphiDigis->chPhiSegm1(wheel,station,sector,bx);
-          dtts2 = inphiDigis->chPhiSegm2(wheel,station,sector,bx - 1 );
+          auto dtts1 = inphiDigis->chPhiSegm1(wheel,station,sector,bx);


The variables are only used within the loop so should be scoped appropriately.

Dr15Jones · 2018-11-18T19:41:37Z

L1Trigger/L1TTwinMux/src/IOPrinter.cc

  const std::vector<L1MuDTChambPhDigi> * vInCon=inphiDigis->getContainer();
-  inphiDigis_tm->setContainer(*vInCon);


This would actually have caused address 0 to be written on (since linke 108 the value is set to nullptr) which would cause a segmentation violation.

Dr15Jones · 2018-11-18T19:42:08Z

L1Trigger/L1TTwinMux/src/L1TTwinMuxAlgorithm.cc

@@ -47,29 +47,29 @@ edm::Handle<L1MuDTChambThContainer> thetaDigis, edm::Handle<RPCDigiCollection> r


  ///Align track segments that are coming in bx-1.
-  auto alignedDTs=std::make_shared<AlignTrackSegments>(*inphiDigis);


The shared_ptrs in this routine only have the lifetime of this function.

Dr15Jones · 2018-11-18T19:43:03Z

L1Trigger/L1TTwinMux/src/RPCtoDTTranslator.cc

 }

 namespace {
  constexpr int max_rpc_bx = 2; 
  constexpr int min_rpc_bx = -2;

+  struct rpc_hit


The struct is only used within the implementation so it is better to encapsulate it here.

Dr15Jones · 2018-11-18T19:43:25Z

L1Trigger/L1TTwinMux/src/RPCtoDTTranslator.cc

+    int strip;
+    int roll;
+    int layer;
+    rpc_hit(int pbx, int pstation,int psector, int pwheel, RPCDetId pdet, int pstrip, int proll, int player) : bx(pbx),station(pstation),sector(psector),wheel(pwheel), detid(pdet),strip(pstrip),roll(proll),layer(player) {}


The constructor is needed for the call to emplace_back.

Dr15Jones · 2018-11-18T19:43:44Z

L1Trigger/L1TTwinMux/src/RPCtoDTTranslator.cc

-        if(detid.station()==3) vrpc_hit_st3.push_back({digi->bx(), detid.station(), detid.sector(), detid.ring(), detid, digi->strip(), detid.roll(), detid.layer()});
-        if(detid.layer()==2) vrpc_hit_layer2.push_back({digi->bx(), detid.station(), detid.sector(), detid.ring(), detid, digi->strip(), detid.roll(), detid.layer()});
-        if(detid.station()==4) vrpc_hit_st4.push_back({digi->bx(), detid.station(), detid.sector(), detid.ring(), detid, digi->strip(), detid.roll(), detid.layer()});
+        if(detid.layer()==1) vrpc_hit_layer1.emplace_back(digi->bx(), detid.station(), detid.sector(), detid.ring(), detid, digi->strip(), detid.roll(), detid.layer());


emplace_back avoids the need to create a temporary and then copy it.

Dr15Jones · 2018-11-18T19:44:02Z

L1Trigger/L1TTwinMux/src/RPCtoDTTranslator.cc

@@ -135,7 +145,7 @@ void RPCtoDTTranslator::run(const edm::EventSetup& c) {
             phi1 = phi1 + (radialAngle(vrpc_hit_layer1[l1-1].detid, c, vrpc_hit_layer1[l1-1].strip));
             phi1 /= 2;
             }
-
+            int itr2 = 0;


This is the one line needed to avoid the crash.

cmsbuild · 2018-11-18T21:27:11Z

+1
Tested at: 29a8a53
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25275/31721/summary.html

cmsbuild · 2018-11-18T21:27:13Z

Comparison job queued.

cmsbuild · 2018-11-19T00:36:46Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25275/31721/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3013311
DQMHistoTests: Total failures: 30
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3013084
DQMHistoTests: Total skipped: 197
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 134 log files, 14 edm output root files, 32 DQM output files

rekovic · 2018-11-20T06:38:03Z

+1

cmsbuild · 2018-11-20T06:38:18Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2018-11-20T09:15:36Z

+1

DQM changes are unrelated to this PR

fabiocos · 2018-11-21T09:15:04Z

@rekovic @thomreis as this is a fix to a crash that is affecting MC productions, could you please backport it to 10_2_X and 10_3_X?

fabiocos · 2018-11-21T15:39:52Z

ok, see #25314 and #25315

Dr15Jones added 11 commits November 18, 2018 10:34

Reset itr2 before call to each inner loop

7eced50

This avoids a crash seen in production. In addition, moved declaration of additional loop variables to be within the scope of the loop.

Do not copy RPCDigiCollection

2e1e2d0

The constructor now holds a const& to the collection.

moved struct rpc_hit to implementation file

d6ae57c

Use emplace back for std::vector<rpc_hit>

bb2e431

Avoid calling function on nullptr

1b118cc

Create a L1MuTMChambPhContainer on the stack rather than having a pointer to null. Also made calling arguments const.

Properly scoped loop internal variables

5f66c38

Made helper classes stack local instead of on the heap

8160364

The helper classes were being made each call and where being allocated via a shared_ptr. Putting them on the stack is more efficient. Also tried to avoid some copies of returned values.

Avoid making unnecessary copies in DTLowQMatching

e32e2e3

Avoid unnecessary copy in RPCHitCleaner

a08e83a

Hold digis by const& to avoid a copy

04bed52

Removed unnecessary use of shared_ptr

29a8a53

Moved the variables to be on the stack. They have the same lifetime as before but can be handled more efficiently.

cmsbuild added this to the CMSSW_10_4_X milestone Nov 18, 2018

cmsbuild added code-checks-pending comparison-pending l1-pending orp-pending pending-signatures tests-pending labels Nov 18, 2018

Dr15Jones mentioned this pull request Nov 18, 2018

L1TTwinMuxProducer:simTwinMuxDigis crashes in SingleMuon Data workflow #25271

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 18, 2018

cmsbuild added tests-started and removed tests-pending labels Nov 18, 2018

Dr15Jones commented Nov 18, 2018

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Nov 18, 2018

cmsbuild added comparison-available and removed comparison-pending labels Nov 19, 2018

cmsbuild added fully-signed l1-approved and removed l1-pending pending-signatures labels Nov 20, 2018

cmsbuild added orp-approved and removed orp-pending labels Nov 20, 2018

cmsbuild merged commit 06afbf8 into cms-sw:master Nov 20, 2018

This was referenced Nov 21, 2018

Backport PR 25275 to 103X: Fix RPCtoDTTranslator #25314

Merged

Backport PR 25275 to 102X: Fix RPCtoDTTranslator #25315

Merged

fabiocos mentioned this pull request Nov 21, 2018

Errors in RPCtoDTTranslator::run #25243

Closed

Dr15Jones deleted the fixRPCtoDTTranslator branch November 22, 2018 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RPCtoDTTranslator #25275

Fix RPCtoDTTranslator #25275

Dr15Jones commented Nov 18, 2018

cmsbuild commented Nov 18, 2018

cmsbuild commented Nov 18, 2018

Dr15Jones commented Nov 18, 2018

cmsbuild commented Nov 18, 2018 •

edited

Loading

cmsbuild commented Nov 18, 2018

Dr15Jones commented Nov 18, 2018

fabiocos commented Nov 18, 2018

Dr15Jones Nov 18, 2018

Dr15Jones Nov 18, 2018

Dr15Jones Nov 18, 2018

Dr15Jones Nov 18, 2018

Dr15Jones Nov 18, 2018

Dr15Jones Nov 18, 2018

Dr15Jones Nov 18, 2018

Dr15Jones Nov 18, 2018

Dr15Jones Nov 18, 2018

cmsbuild commented Nov 18, 2018

cmsbuild commented Nov 18, 2018

cmsbuild commented Nov 19, 2018

rekovic commented Nov 20, 2018

cmsbuild commented Nov 20, 2018

fabiocos commented Nov 20, 2018

fabiocos commented Nov 21, 2018

fabiocos commented Nov 21, 2018

		@@ -41,16 +41,16 @@ void DTRPCBxCorrection::run( const edm::EventSetup& c) {

		void DTRPCBxCorrection::BxCorrection(int track_seg){

		auto m_phiDTDigis_tm=std::make_shared<L1MuTMChambPhContainer>();

		const std::vector<L1MuDTChambPhDigi> * vInCon=inphiDigis->getContainer();
		inphiDigis_tm->setContainer(*vInCon);

		@@ -47,29 +47,29 @@ edm::Handle<L1MuDTChambThContainer> thetaDigis, edm::Handle<RPCDigiCollection> r


		///Align track segments that are coming in bx-1.
		auto alignedDTs=std::make_shared<AlignTrackSegments>(*inphiDigis);

Fix RPCtoDTTranslator #25275

Fix RPCtoDTTranslator #25275

Conversation

Dr15Jones commented Nov 18, 2018

cmsbuild commented Nov 18, 2018

cmsbuild commented Nov 18, 2018

Dr15Jones commented Nov 18, 2018

cmsbuild commented Nov 18, 2018 • edited Loading

cmsbuild commented Nov 18, 2018

Dr15Jones commented Nov 18, 2018

fabiocos commented Nov 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Nov 18, 2018

cmsbuild commented Nov 18, 2018

cmsbuild commented Nov 19, 2018

rekovic commented Nov 20, 2018

cmsbuild commented Nov 20, 2018

fabiocos commented Nov 20, 2018

fabiocos commented Nov 21, 2018

fabiocos commented Nov 21, 2018

cmsbuild commented Nov 18, 2018 •

edited

Loading